Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to downsample on bake in LightmapGI #93445

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 21, 2024

This provides increased lightmap quality with less noise, smoother shadows and better small-scale shadow detail. The downside is that this significantly increases bake times and memory usage while baking lightmaps, so this option is disabled by default.

I've experimented with allowing the use of downsampling factors other than 2.0 (on each axis), but they didn't seem to improve quality significantly (other than reducing noise due to the ray count automatically becoming higher). It seems we can't get much smoother shadows, other than by using bicubic sampling. Maybe there's an issue about Image::resize() not being sRGB-aware, using the wrong gamma or something similar – especially on HDR formats.

I've also tried various image resizing algorithms and settled on bilinear, as cubic tended to make shadows more aliased (and Lanczos even more so). Trilinear would be useful for downsampling factors greater than 2.0 still.

cc @lander-vr @passivestar

Testing project: test_lightmapgi_downsample.zip

Preview

Texel Scale 2.0, no downsampling

No downsampling

Texel Scale 2.0, with downsampling

With downsampling

Texel Scale 16.0, no downsampling (for reference)

Consider this as "ground truth", although some elements look different such as the ambient occlusion in crevices.

8× Texel Scale

@jcostello
Copy link
Contributor

Very nice. Downscalling to much could potential crash the baking due to #54679 ?

@Calinou
Copy link
Member Author

Calinou commented Jun 21, 2024

Very nice. Downscalling to much could potential crash the baking due to #54679 ?

Yes, it's another reason why we might not want to expose downsampling factors higher than 2.0 right now.

@jcostello
Copy link
Contributor

Probably is a good time to address that issue. It bringing too much problems to work with lightmaps. We have to limit to small scenes or low quality bakes because of it

@passivestar
Copy link
Contributor

passivestar commented Jun 22, 2024

Texel Density 5, Downsampling Off:

5-nods

Texel Density 5, Downsampling On:

5-ds

Texel Density 2, Downsampling Off:

2-nods

Texel Density 2, Downsampling On:

2-ds

Huge improvement at texel density 2, at higher texel density returns do be diminishing, but it still managed to clean some things up at texel density 5 (i.e black spots on the tire of the car are almost entirely gone)

Notice that when using downsampling with texel density 5 some objects have thin dark lines around edges (easiest to spot on the tree on the right, but you can also see them on bricks on top of the stack of bricks and on the broken concrete barrier)

At density 2 it also improved the situation with jagged lines on the rocks on the right and on the tree on the left

I've experimented with allowing the use of downsampling factors other than 2.0 (on each axis), but they didn't seem to improve quality significantly

I wonder if a slightly bigger factor would eliminate the remaining pitch black spots on the trucks (in both 2 and 5 texel density scenarios)

@passivestar
Copy link
Contributor

passivestar commented Jun 22, 2024

I wonder if a slightly bigger factor would eliminate the remaining pitch black spots on the trucks (in both 2 and 5 texel density scenarios)

I checked with a factor of 3 (at texel density 2), the truck got much cleaner, almost perfect:

Screenshot 2024-06-22 at 20 26 22

However trees got worse:

Screenshot 2024-06-22 at 20 26 33

So if the situation with the dark edges can be figured out (maybe it's an unrelated bug?) I'd like to have an option to go higher than 2 because it helps with tiny faces

If it can't be figured out I'd like that option all the same, because dark edges are much less visible than super-dark face interiors that tiny faces cause

@lander-vr
Copy link
Contributor

Really excited to see this, thanks for this @Calinou! Unfortunately I don't have a lot of time to test this out myself at the moment, hopefully I can get around to it sometime in the coming week.

@passivestar

Huge improvement at texel density 2, at higher texel density returns do be diminishing

This would likely be dependent on the texel density paired with how intricate the geometry is, which also seems to be what you're experiencing with getting better results on tiny faces when you go over 2.

I am wondering if downsampling is the best/most intuitive term for this feature though. I think the important part that users should understand is that the feature results in a higher quality. I feel like downsampling may confuse people, possibly expecting it would result in a lower resolution lightmap, and not expecting the increased render times. It's descriptive of how it works under the hood, but in my opinion maybe not intuitive for its output (i.e. a higher quality bake at the same resolution).

Maybe something like "quality scale" would be more intuitive?

@passivestar
Copy link
Contributor

I am wondering if downsampling is the best/most intuitive term for this feature though

This crossed my mind too, something that's starting with "down" doesn't really associate with an improvement in my mind, but I thought that it's probably because I don't understand the term. So I googled it and turns out that's the case, downsampling is the correct technical term for this

Personally I would prefer "supersampling" because it makes it more obvious it'll render a bigger image

@lander-vr
Copy link
Contributor

downsampling is the correct technical term for this

It is but only for a part of the feature, the two crucial bits are that we first render at e.g. 2x the resolution, and then downsample it back down to the desired texel density. That said, I don't think the name should necessarily convey what it does under the hood. I think for users it's more important to understand what it achieves, rather than how it achieves it.

Supersampling is unfortunately already well known as an anti-aliasing technique, so I'm afraid that could also give an inaccurate idea of what this feature achieves.

This provides increased lightmap quality with less noise, smoother
shadows and better small-scale shadow detail. The downside is that
this significantly increases bake times and memory usage while baking
lightmaps, so this option is disabled by default.
@Calinou Calinou force-pushed the lightmapgi-add-downsampling branch from e6ef548 to c568c47 Compare August 20, 2024 22:57
@Calinou
Copy link
Member Author

Calinou commented Aug 20, 2024

Rebased and tested again, it works as expected.

However, when combined with #95828 (which I tested locally), aliasing becomes more visible when downsampling is enabled:

It seems none of the Image resizing algorithms perform good antialiasing when downsampling, not even Cubic or Bilinear. I would expect this to occur based on what happens when you downscale images in GIMP, ImageMagick or similar. This issue might be limited to HDR formats, which lightmaps use. Calling texture_image->generate_mipmaps() before texture_image->resize() in lightmap_gi.cpp doesn't improve quality.

Texel Scale is set to 6.0. Bicubic filtering in lightmap rendering is disabled for the purposes of comparison here, but the issue still applies when it's enabled.

No downsampling

No downsampling

Nearest downsampling

Nearest downsampling

Bilinear downsampling

Bilinear downsampling

Cubic downsampling

Cubic downsampling

Lanczos downsampling

Lanczos downsampling

@passivestar
Copy link
Contributor

Not sure about the aliasing problem but it still would be nice to get an opinion on making the downsampling factor user-configurable because the difference appears to be tangible on some narrow faces

ds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants